Feature: Adding Rhel10 Base Support using dnf4 package manager#359
Feature: Adding Rhel10 Base Support using dnf4 package manager#359yashnap wants to merge 9 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #359 +/- ##
==========================================
+ Coverage 94.04% 94.17% +0.12%
==========================================
Files 109 111 +2
Lines 19967 20964 +997
==========================================
+ Hits 18778 19742 +964
- Misses 1189 1222 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds RHEL 10 support to the LinuxPatchExtension by introducing a DNF4-based package manager implementation and wiring it into package-manager detection and configuration, along with test/mocking updates to cover RHEL10 scenarios.
Changes:
- Introduces
Dnf4PackageManagerwith update discovery, dependency parsing, reboot detection, and auto-OS-update disable/revert logic. - Updates
EnvLayer+ConfigurationFactoryto detect and instantiate the new DNF4 flow on RHEL 10. - Adds/updates unit tests and legacy env-layer command mocks to simulate DNF4/RHEL10 behaviors.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/references/cmd_output_references/dnf4_ouput_expected_formats | Adds reference examples of DNF4 output formats used by parsers/tests. |
| src/core/tests/Test_EnvLayer.py | Updates env-layer tests/mocks to reflect RHEL10 detection and DNF version probing. |
| src/core/tests/Test_Dnf4PackageManager.py | Adds a new test suite for DNF4 behavior (repo refresh, dependency simulation, auto OS update config, etc.). |
| src/core/tests/Test_CoreMain.py | Adds an autopatching test covering RHEL10 + DNF4 behavior. |
| src/core/tests/library/LegacyEnvLayerExtensions.py | Extends the legacy command-output mocking to emulate DNF4 outputs and systemctl/rpm behaviors. |
| src/core/src/package_managers/Dnf4PackageManager.py | New package-manager implementation for DNF4/RHEL10. |
| src/core/src/bootstrap/EnvLayer.py | Adds RHEL10 path to select DNF4 based on dnf --version. |
| src/core/src/bootstrap/Constants.py | Adds Constants.DNF4. |
| src/core/src/bootstrap/ConfigurationFactory.py | Wires DNF4 into DI configurations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'apt_dev_config': self.new_dev_configuration(Constants.APT, AptitudePackageManager), | ||
| 'dnf4_dev_config': self.new_prod_configuration(Constants.DNF4, Dnf4PackageManager), | ||
| 'dnf5_dev_config': self.new_dev_configuration(Constants.DNF5, Dnf5PackageManager), |
| 'apt_test_config': self.new_test_configuration(Constants.APT, AptitudePackageManager), | ||
| 'dnf4_test_config': self.new_prod_configuration(Constants.DNF4, Dnf4PackageManager), | ||
| 'dnf5_test_config': self.new_test_configuration(Constants.DNF5, Dnf5PackageManager), |
| def validate_dnf4_output(self, output): | ||
| for failure_text in self.dnf4_subscription_failure_texts: | ||
| if failure_text in output: | ||
| self.composite_logger.log_error("[DNF4] Subscription/entitlement failure detected. [{0}]".format(failure_text)) | ||
| raise Exception("System is not properly registered with subscription service.") |
| elif cmd.find("systemctl") > -1: | ||
| code = 1 | ||
| output = '' | ||
| elif self.legacy_package_manager_name is Constants.DNF4: |
| self.assertEqual(len(available_updates), 0) | ||
| self.assertEqual(len(package_versions), 0) | ||
|
|
||
| def test_install_package_failure(self): |
| # Restart not required (needs-restarting returns code=0) | ||
| self.runtime.set_legacy_test_type('SadPath') | ||
| self.runtime.env_layer.run_output_command = self.mock_run_command_output_no_reboot | ||
| self.assertFalse(package_manager.is_reboot_pending()) |
| security_packages, security_package_versions = package_manager.get_security_updates() | ||
| self.assertTrue(5, security_packages) | ||
| self.assertTrue(5, security_package_versions) |
| def test_update_os_patch_configuration_sub_setting_exception_handling(self): | ||
| """Test exception handling when override file write fails""" | ||
| self.runtime.set_legacy_test_type('HappyPath') | ||
| package_manager = self.container.get('package_manager') | ||
| # Mock file_system.write_with_retry to raise exception | ||
| self.runtime.env_layer.file_system.write_with_retry = self.mock_write_with_retry_raise_exception | ||
| self.assertRaises(Exception, package_manager.update_os_patch_configuration_sub_setting, ) |
michellemcdaniel
left a comment
There was a problem hiding this comment.
As a general note, you have varying spacing in some of your function descriptions, i.e. sometimes including a space or not after """ at the beginning of the description and before the ending """ and sometimes excluding those spaces. Please go through and standardize one way or the other for consistency. Same with function calls and including spaces after commas or not. In that case, please include the space.
|
|
||
| def __get_dnf_version(self): | ||
| code, out = self.run_command_output('dnf --version', False, False) | ||
| # Output : dnf5 version 5.2.18.0/ |
There was a problem hiding this comment.
is the / at the end of this comment intended?
There was a problem hiding this comment.
No, earlier I had dnf5 version 5.2.18.0/4.20.0 but updated later. Removed / from the end
| error_msg = "This distro is not yet supported in your region. Please review https://aka.ms/VMGuestPatchingCompatibility for more information. [Distro={0}][Version={1}][Code={2}]".format(str(os_name), os_version, os_code) | ||
| print("Error: {0}".format(error_msg)) | ||
| if not self.__is_dnf_available(): | ||
| print("Error: Expected package manager dnf not found on this rhel 10 VM.") |
There was a problem hiding this comment.
Would prefer if you matched the original error message formatting.
| if version: | ||
| if version.startswith('4'): | ||
| return Constants.DNF4 | ||
| print("Error: Expected dnf version 4 on this rhel 10 VM. Found: {0}".format(version)) |
| @@ -93,8 +108,16 @@ def get_package_manager(self): | |||
|
|
|||
| # Check for unsupported distros | |||
There was a problem hiding this comment.
Update comment. Rhel10 no longer unsupported.
| architectures = Constants.SUPPORTED_PACKAGE_ARCH | ||
| for arch in architectures: | ||
| if package_name.endswith(arch): | ||
| return package_name[:-len(arch)], arch |
There was a problem hiding this comment.
earlier, you noted that the package name would be in the form .. Could we not split on . and return split[0], split[1] (assuming a known arch is found)?
There was a problem hiding this comment.
Yes, from the current evidence it looks like packageName.arch and we can split on the first .

However, since this is new to me as well so the question would be if we will have other packages that has multiple dots i.e python-3.10.module.arch in which case the first dot approach would produce incorrect results. (The current one would work regardless)
There was a problem hiding this comment.
Makes sense. I hadn't considered packages that may have extraneous periods in the package name. Might not even be safe to split on period and take the last chunk would it?
There was a problem hiding this comment.
One last thing: I think we might need to do len(arch)+1 here. Can you make sure that we are not accidentally returning "package_name." instead of "package_name" (note the additional period at the end). I did a little test with:
text= "hello.world"
substring = text[:-len("world")]
print(substring)and it print out "hello." So we need to make sure we strip out the trailing period
| if not patch_configuration_sub_setting_found_in_file: | ||
| updated_patch_configuration_sub_setting += patch_configuration_sub_setting_to_update + "\n" | ||
|
|
||
| self.env_layer.file_system.write_with_retry(self.os_patch_configuration_settings_file_path,'{0}'.format(updated_patch_configuration_sub_setting.lstrip()),mode='w+') |
There was a problem hiding this comment.
is there something weird about the spacing on this line or is github lying to me? It may be the lack of spaces in the line. Let's add spaces after each comma
There was a problem hiding this comment.
No it's the spacing after comma. I've added it
| apply_updates_value_from_backup = image_default_patch_configuration_backup[self.current_auto_os_update_service][self.apply_updates_identifier_text] | ||
| enable_on_reboot_value_from_backup = image_default_patch_configuration_backup[self.current_auto_os_update_service][self.enable_on_reboot_identifier_text] | ||
|
|
||
| self.update_os_patch_configuration_sub_setting(self.download_updates_identifier_text,download_updates_value_from_backup,self.auto_update_config_pattern_match_text) |
| if str(enable_on_reboot_value_from_backup).lower() == 'true': | ||
| self.enable_auto_update_on_reboot() | ||
| else: | ||
| self.composite_logger.log_debug("[DNF4] Since the backup is invalid or does not exist for current service, we won't be able to revert auto OS patch settings to their system default value. [Service={0}]".format(str(self.current_auto_os_update_service))) |
There was a problem hiding this comment.
Since this is logging that will end up in customer logs, let's reword this to something like "Backup is invalid or does not exist for current service. Unable to revert auto OS patch settings to system default value."
I know that sounds very similar, but the pronouns feel weird in this sort of logging.
There was a problem hiding this comment.
I've updated the message
| def __get_image_default_patch_configuration_backup(self): | ||
| """ Get image_default_patch_configuration_backup file""" | ||
| image_default_patch_configuration_backup = {} | ||
| # read existing backup since it also contains backup from other update services. We need to preserve any existing data within the backup file |
@michellemcdaniel I've updated the function descriptions to remove spacing from the """start as well as the end""" to keep it consistent. Also added space after , that was missing at multiple places. Somehow when I copy paste code in Pycharm it automatically adds new lines and when putting it back on one the spaces are missed. I think its good now |
| os_name, os_version, os_code = self.platform.linux_distribution() | ||
|
|
||
| # Check for unsupported distros | ||
| # Check for Rhel 10 ( uses dnf4) |
There was a problem hiding this comment.
nit: Fix the spacing in this comment
| architectures = Constants.SUPPORTED_PACKAGE_ARCH | ||
| for arch in architectures: | ||
| if package_name.endswith(arch): | ||
| return package_name[:-len(arch)], arch |
There was a problem hiding this comment.
One last thing: I think we might need to do len(arch)+1 here. Can you make sure that we are not accidentally returning "package_name." instead of "package_name" (note the additional period at the end). I did a little test with:
text= "hello.world"
substring = text[:-len("world")]
print(substring)and it print out "hello." So we need to make sure we strip out the trailing period
Implemented Dnf4PackageManager by extending PackageManager.
Implemented changes:
TESTS
On demand Assessment (ConfigurePatching can be validated within Assess or Install Patches run)
4.core.log
On demand Installation, Classification: [Critical, Security, Other]
"classificationsToInclude": ["Security","Other","Critical"]
5.core.log
Auto assessment, recurring on schedule - In Progress
Only Package inclusions installed
7.core.log
Included : python3-perf ( Only installed)
With package exclusions i.e. excluded packages are not installed
Exclude list [xxd, openssl ] - Both not installed
4.core.log
With Dependent packages i.e. dependent packages identified and installed
6.core.log
Included : coreutils (It installed dependent packages coreutils-common etc)
Excluding a package because its dependency needs to be excluded : I
5.core.log
Included: fprintd , Excluded : fprintd-pam
Auto Patching request with only security and critical updates in request, which should install all classifications
"classificationsToInclude": ["Security", "Critical"]
9.core.log
Logs for disabling auto OS (machine default) updates
9. Machine default updates service installed but NOT enabled - In Progress
Machine default updates service NOT installed - In Progress
Machine default updates service installed and enabled - ConfigurePatching reads and logs that auto OS updates are installed and enabled and disables them. Auto OS updates are disabled
8.core.log
**Note on Redhat machine not being able to get updates with the below message: **
Unable to read consumer identity This system is not registered with an entitlement server. You can use "rhc" or "subscription-manager" to register.Add Multi_arch_dependencies:
Evaluated whether DNF4 needs add_arch_dependencies() logic. Verified that DNF4 already expands transactions automatically during dependency resolution.
Could not find any package in RHEL10 repos that exists with:
same package name
same version
different architecture
No evidence found that DNF4 requires manual architecture sibling expansion.
Validation Performed
Ran:
dnf4 update glibc.x86_64 --assumenoDNF4 automatically added:
glibc-common
glibc-gconv-extra
glibc-langpack-en
Ran:
dnf4 update kernel.x86_64 --assumenoDNF4 automatically added:
kernel-core
kernel-modules
kernel-modules-core
Checked multilib configuration:
multilib_policy = best
Searched for packages available in multiple architectures.
Verified versions of those packages. Architectures existed, but versions were different.
Example:
cockpit-bridge.noarch 356.2-1.el10_2
cockpit-bridge.x86_64 334.1-1.el10_0
Performed repository-wide scan for same package name, same version and multiple architectures but no matches found
Thoughts:
DNF4 already performs dependency/transaction expansion internally.
Could not reproduce the exact scenario that add_arch_dependencies() was designed for
No evidence found that DNF4 requires additional architecture expansion logic at this time.